fix(cli): keep keypress handlers current - #5421

@qwen-code /triage

updated the PR description to match the template. code is unchanged.

@qwen-code /triage

Thanks for the PR!

Template looks good ✓ — all required sections present, bilingual, test plan included.

On direction: this fixes a real timing bug where AskUserQuestionDialog could receive keypress events through a stale callback during the render→effect gap on macOS. Keyboard input correctness in the CLI is squarely within core mission. Aligned.

On approach: the scope is tight — +55/-11 across two files. The fix uses the idiomatic React pattern (ref for latest handler + stable useCallback wrapper) to keep the subscription identity constant. The test changes replace fragile fixed wait(150) delays with a polling helper. Every line in the diff serves the stated goal; nothing to cut.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code Review

Independent proposal (before reading diff): The problem is stale keypress handlers during the render→effect window. The idiomatic fix is to store the handler in a ref updated on each render, and subscribe/unsubscribe with a stable wrapper via useCallback(fn, []). For the test, replace fixed wait() delays with a polling helper that retries assertions.

Comparison with PR: The PR's approach matches my proposal exactly. The implementation is clean:

• useRef(onKeypress) + onKeypressRef.current = onKeypress during render — standard React pattern, safe for this use case
• useCallback<KeypressHandler>((key) => onKeypressRef.current(key), []) — stable identity, never re-subscribes
• Effect deps [isActive, handleKeypress, subscribe, unsubscribe] — correct; handleKeypress is stable so the effect only re-runs on isActive toggle

The waitForFrame test helper is well-written — retries a predicate every 10ms for up to 1s, properly tracks the last error, and has a clear timeout message. This also makes the tests more robust in general, not just for this specific flake.

No blockers found. No correctness issues, no security concerns, no AGENTS.md violations. The code is minimal, idiomatic, and well-scoped.

Test Results

Unit tests for both affected files pass on PR branch and on main:

PR branch:
 ✓ src/ui/hooks/useKeypress.test.ts (15 tests) 55ms
 ✓ src/ui/components/messages/AskUserQuestionDialog.test.tsx (18 tests | 1 skipped) 1641ms
 Tests  32 passed | 1 skipped (33)

main:
 ✓ src/ui/hooks/useKeypress.test.ts (15 tests) 52ms
 ✓ src/ui/components/messages/AskUserQuestionDialog.test.tsx (18 tests | 1 skipped) 2014ms
 Tests  32 passed | 1 skipped (33)

ESLint: clean. Prettier: clean.

Real-Scenario Testing

This fix targets a sub-frame React hook subscription timing issue that manifests only under specific React scheduling conditions on macOS CI. It's not reproducible through manual TUI interaction — the timing window is between a render pass and the subsequent useEffect cleanup/setup cycle, which is invisible to a human user typing at normal speed. The unit tests, particularly the tightened AskUserQuestionDialog regression test with waitForFrame, are the appropriate verification layer for this class of bug.

— Qwen Code · qwen3.7-max

This is a clean, well-scoped fix for a real timing bug. The ref+stable-callback pattern is exactly what I'd write independently. The test improvement (replacing fixed waits with polling) is a genuine quality upgrade — it makes the test both faster and more robust.

Two files, 55 additions, 11 deletions, zero waste. Lint, format, and all 32 tests pass. The prior CHANGES_REQUESTED review may have been on an earlier revision; the current code is solid.

Approving. ✅

— Qwen Code · qwen3.7-max

✅ Local verification — PR #5421 fix(cli): keep keypress handlers current

Verdict: LGTM, safe to merge. I built the PR on top of main and verified the fix mechanism deterministically at the hook level, then drove the real AskUserQuestionDialog (real KeypressProvider/useKeypress) through a real tmux PTY.

Note on the merge surface: the branch carries two commits — the keypress fix plus a duplicate of the cron commit (#5230) that is already in main — so GitHub's diff (and the real merge) is exactly the 2 files. I cherry-picked only the keypress commit; both changed files are byte-identical between the branch's base and current main (0-line diff), so this is precisely the net merge.

What the PR changes

1. useKeypress.ts — subscribe a stable wrapper once and read the live handler via onKeypressRef.current, instead of subscribing the onKeypress closure directly (which forced an unsubscribe + re-subscribe on every render). Classic latest-ref / "useEvent" pattern.
2. AskUserQuestionDialog.test.tsx — replace fixed await wait(150) with a polling waitForFrame(...), and add stronger assertions (after j the frame contains j; after k it contains jk).

The bug it targets: in the dialog, bare j/k are vim nav (SELECTION_DOWN/UP) in the normal branch but must be typed once "Other" is selected. A handler that goes stale between renders still thinks "Other" isn't selected, so a typed j/k gets stolen as cursor navigation.

1 · Tests green, no flakes

• useKeypress.test.ts + AskUserQuestionDialog.test.tsx → 32 passed, 1 skipped.
• Full dialog suite run 5× → 17 passed / 1 skipped every time. Stable.

2 · Deterministic A/B of the fix mechanism (hook level)

Mocked the dispatcher's subscribe/unsubscribe, mounted useKeypress with an inline handler closing over value, captured the function the dispatcher received at mount, then re-rendered with value=2 and invoked that original registration:

The base re-subscribes on every render, and the function the dispatcher holds goes stale in the window between render-commit and the effect re-running. The PR subscribes once, and that single registration always calls the latest closure via the ref. That window is the real-terminal win: a keypress arriving there hits a stale handler on base, a current one on the PR.

3 · Important nuance — the new test passes on the unfixed hook too

I ran the PR's own regression test (keeps bare k/j in custom input while Ctrl+P/N still navigates options) against the base hook 10× → 10/10 pass. Because the new waitForFrame + await wait() give the base hook's effect time to re-subscribe between keystrokes, the test does not fail on the unfixed hook. So:

• the test change de-flakes the previously-flaky test (that's the real value of the polling), and
• the hook change fixes the real-terminal race that a key-by-key awaited test can't reproduce.

Both are good; they fix different layers.

4 · Real terminal (tmux PTY) — real component + real useKeypress (PR)

4    →  ❯ 4. > Type something...      (Other selected, custom input active)
j    →  ❯ 4. > j                      (TYPED, not consumed as vim-down)
k    →  ❯ 4. > jk                     (TYPED)
C-p  →  ❯ 3. Green   |  4. jk          (Ctrl+P navigates up, value preserved)
C-n  →  ❯ 4. > jk                      (Ctrl+N back to custom input)

Exactly the contract in the test name — end-to-end through the real keypress pipeline.

5 · Honesty note — the raw race is not cleanly tmux-reproducible

Firing 4jk as a single un-paced write lands on ❯ 1. Red with an empty custom input on both base and PR — because no React render happens between the bytes, so even the PR's ref can't update. Paced input works on both. The base↔PR difference lives only in the narrow render→effect window, which send-keys timing can't reliably hit — which is why §2's hook-level A/B is the proper proof, not a tmux A/B.

6 · Regression sweep (other useKeypress consumers)

useKeypress + TextInput + RadioButtonSelect + AskUserQuestionDialog → 43 passed.
(InputPrompt.test.tsx fails to load in my sandbox on an unrelated ink@7.0.3 Missing "./dom" specifier — pre-existing dep skew via BaseTextInput, fails identically with and without the PR.)

Minor, non-blocking

• The ref is assigned in the render body (onKeypressRef.current = onKeypress). Fine for Ink (synchronous renderer, no StrictMode double-render); if the TUI ever moves to concurrent React, prefer assigning it in useLayoutEffect.
• As shown in §3, the shipped integration test guards the timing flake but passes on the unfixed hook — a hook-level test like the §2 probe would be a stronger regression guard for the handler-currency behavior itself. Optional.